-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use dynamic-cdn-webpack-plugin for react and react-dom #3914
Conversation
@@ -376,6 +377,11 @@ module.exports = { | |||
minifyURLs: true, | |||
}, | |||
}), | |||
new DynamicCdnWebpackPlugin({ | |||
only: ['react', 'react-dom'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it work when the app is cached using service worker and the network is offline? IMO this needs to be opt-in if we want to move forward with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a decision the maintainers should take. we already discussed the key motivations in #2758. @gaearon and others are in favor of using CDNs in general, but I understand your concern.
as far as SW and CRA are concerned I know we are moving towards making them opt-in: #2554. its an easier call there because its just commenting out one line of code. but still. we're moving away from it for now.
IMO CRA should cater to the 90% use case as we always have eject as an option. thats what its for. as far as i understand it, service workers are not in the 90% use case for CRA (this is why we keep getting so many issues about it)
as a last resort: if we really want to we can check for something like process.env.CDNModules
to opt out of it for build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea but if people chose to opt-in for SW, it broke their expectation because the created app is no longer working offline.. I think it should be a choice between cdn capable vs offline capable. Or maybe we can configure sw-precache
to cache the files from cdn?
This is great but I agree with @viankakrisna that this functionality should be opt-in. I'm not sure about the best way to achieve that though. |
maybe something like |
so can this feature be used now? @sw-yx |
@chj-damon I've tested it and its usable, but @iansu and @viankakrisna want to do this through opt in instead of by default. so I'm not sure how they want to proceed. |
Hi
in connection with the community suggestion here: #2758
this is how it would work in react-scripts. i have only implemented this for production as I think it's probably not needed (possibly might cause some confusion if developing react at the same time) in development.
This PR results in the CRA bundle size going from 36 kb:
to 6kb (pardon the edited App.js text, i did not commit that lol):
and TTI should improve in actual deployed situation due to parallelized loading (not to mention caching benefits).
I have erred on the side of verbose with the comments for CRA users who may be unfamiliar with this plugin but am open to taking them out.